Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: fix main session OOPIF checks for devtools #12533

Merged
merged 3 commits into from
May 25, 2021

Conversation

connorjclark
Copy link
Collaborator

I noticed in DevTools that no source map contents were being used: so no subRows in unused-javascript with module-level coverage, and no bundle in script-treemap-data.

JSBundles had errored sizes because scriptElement.content was null:

image

This property is set for external scripts in a post-processing step in ScriptElements:

image

In DevTools, the OOPIF check filtered out all records because sessionId is set even for the main protocol connection. This is due to a complication introduced while adding multi-client support. See https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2607769 for more. I failed to followup on that, and we have no tests touching OOPIF checks in devtools tests, so it went unnoticed.

This PR is a possible workaround/bandaid. I manually verified with yarn open-devtools that JSBundles sizes property is set correctly, and all the audits are correctly processing module information. I'll look into adding tests too.

@connorjclark connorjclark requested a review from a team as a code owner May 21, 2021 18:39
@connorjclark connorjclark requested review from adamraine and removed request for a team May 21, 2021 18:39
@google-cla google-cla bot added the cla: yes label May 21, 2021
// Many places in Lighthouse use `record.sessionId === undefined` to mean that the session is not
// an OOPIF. To maintain this property, we intercept sessionId here and set it to undefined if
// it matches the first value seen.
if (this._mainSessionId === null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish the roles of undefined and null are reversed for _mainSessionId, but doing so would probably require changes to the protocol and/or large changes to how we handle sessionId downstream. Sounds like a lot of work for a minor annoyance, so this LGTM.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll need to take another look at the entire use of sessionId when we revisit FR OOPIF handling which is going to have to be more complex anyway. I'm also fine with a temporary bandaid here

@connorjclark connorjclark changed the title core(network-recorder): fix main session OOPIF checks for devtools core: fix main session OOPIF checks for devtools May 25, 2021
@connorjclark connorjclark merged commit d882612 into master May 25, 2021
@connorjclark connorjclark deleted the devtools-main-session branch May 25, 2021 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants